Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding flask mqtt example that uses tls/ssl #117

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lefty01
Copy link

@lefty01 lefty01 commented Nov 18, 2022

tls example with test.mosquitto.org certs

@Sohaib90
Copy link
Collaborator

Hi
Can you look into the conflicting files and maybe resolve them?
I will look into the PR over the weekend :)

@lefty01
Copy link
Author

lefty01 commented Jan 12, 2023

Hey ... cool .... it's been a while ;)
so I basically moved the previous example into an extra dir and added one additional directory for the ssl example.
merged in your latest main and also copied the requirements file (forgot that one).
hope this is ok with u?

@Sohaib90
Copy link
Collaborator

Sohaib90 commented Jan 12, 2023

Apologies for the delay. I will look into the PR over the weekend and update things here :)

Just a quick comment:
Dont you think all the examples should have a single environment.yml and requirements.txt . Or do they have some differences? Moreover, can you discuss why you think different directories are essential in the example folder when they can also have descriptive file names? :)

@lefty01
Copy link
Author

lefty01 commented Jan 12, 2023

well at first I thought they might differ in some way (like extra ssl module) but yes they are equal right now and there's is no need to separate as it looks right now. it's just that personally I think would keep it separate in case some other example would be added which might have different module requirements.

but even then I think it can be argued that for the examples we can have a single requirements file that would include everything needed by all examples.
thinking about it now .... I think either way would be ok. so I would leave it up to you :)
if you prefer single example directory I'll change that n/p. I can see the point that it would make the change a bit smaller as well.

@Sohaib90
Copy link
Collaborator

I think having a smaller change is better so we can have only one requirements and environment files for all the examples. That will allow for simpler change and it will be simple for anyone who uses it as well. So, I would suggest that we have only one requirements.txt and environment.yml and also have different examples differ by only the filename rather than make new directories :)

@lefty01
Copy link
Author

lefty01 commented Jan 13, 2023

next round ;)
... how about the venv name in environment.yml ... it has/had your user home hardcoded
removed trailing whitespace in template file

@lefty01
Copy link
Author

lefty01 commented Jan 13, 2023

hrmm ... well I admit I have not used conda ... so not sure how / if it works.
I would do something like:

$ mkvirtualenv -r requirements.txt flask_mqtt_example

this however lead to a few problems with some of the version ... I have updated (not pushed) the requirements.txt file.

which conda command would you be using? (if you are using it ... or otherwise how do u use the environment.yaml file?)

@Sohaib90
Copy link
Collaborator

next round ;) ... how about the venv name in environment.yml ... it has/had your user home hardcoded removed trailing whitespace in template file

I think changing the name in the environment.yml file is a good edit. That was set up by the original author of the repo.

@Sohaib90
Copy link
Collaborator

hrmm ... well I admit I have not used conda ... so not sure how / if it works. I would do something like:

$ mkvirtualenv -r requirements.txt flask_mqtt_example

this however lead to a few problems with some of the version ... I have updated (not pushed) the requirements.txt file.

which conda command would you be using? (if you are using it ... or otherwise how do u use the environment.yaml file?)

To use the environment.yml file you can use conda. You can look into how to use conda for that here:

https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#creating-an-environment-from-an-environment-yml-file

You can test if the version problems occur here as well and then update both simultaneously? :)

@lefty01
Copy link
Author

lefty01 commented Jan 13, 2023

thx
will check that later aka tomorrow

@Sohaib90
Copy link
Collaborator

Thank you so much for the work :)

@Sohaib90
Copy link
Collaborator

Sohaib90 commented Jan 13, 2023

I want to add one more comment.
If you look here

# Parameters for SSL enabled

Code from line 32-36 adds the hint for adding tls/ssl. Just for discussion, how do you think your example needs a separate example file? I was looking into the master app.py and was wondering

@lefty01
Copy link
Author

lefty01 commented Jan 14, 2023

I want to add one more comment. If you look here

# Parameters for SSL enabled

Code from line 32-36 adds the hint for adding tls/ssl. Just for discussion, how do you think your example needs a separate example file? I was looking into the master app.py and was wondering

oh .. well another good question.
I might not have looked to close at that time.
But since I use a public broker where I also set it up to request client certificate I think that was at least one thing that was not in that section. yeah again now that renders this PR almost useless ;)
Maybe we could control it via defines or something, maybe not everybody would want to use client certs?
also MQTT_TLS_INSECURE might differ.
In that sense even my example is not the best as tls has a few options to choose from.

so I could think of guarding that section with something like
#ifdef USE_SSL and #ifdef USE_CLIENT_CERT
and then keep only one .py file
... would that be acceptable ?

@Sohaib90
Copy link
Collaborator

One idea of doing this can be that you add the logic to the existing app.py and maybe comment it out like it has already been done. This way whoever wants to use it can just un-comment your lines and use that. This way we wont have two identical scripts with just one different code block. What do you think about that?

On top of that, you can also add a section in the README.md that explains this part so that if someone is looking for a bit of explanation, they can easily access it in the documentation. Does that sound okay? You can also discuss otherwise

@lefty01
Copy link
Author

lefty01 commented Jan 19, 2023

will check this .... hold on ;)

@Sohaib90
Copy link
Collaborator

Sohaib90 commented Oct 1, 2023

@lefty01 any update on this PR you created?

@lefty01
Copy link
Author

lefty01 commented Oct 1, 2023

gee .. yeah that was dangling around for a long time.
I just updated the PR and added commented code to the existing app.py example and refllected that in the README.
In addition added client.key to .gitignore and removed trailing whitespace from two line in index.html.
so feel free to merge, reject or otherwise comment ;)

@Sohaib90
Copy link
Collaborator

Looks fine to me @lefty01. I think you fixed some issues. I would like to ask you to add some test cases to support your PR

@Sohaib90
Copy link
Collaborator

Looks fine to me @lefty01. I think you fixed some issues. I would like to ask you to add some test cases to support your PR

After that I think this is good to go :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants